Skip to content

Conversation

@GuySten
Copy link
Contributor

@GuySten GuySten commented Sep 25, 2025

Description

Currently the reference direction for the azimutal angle in PolarAzimuthal distribution is not clear.

This PR enable specifying the direction in the python API so it is clear what is going on and what is the direction that corresponds to a specific phi

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@GuySten GuySten marked this pull request as ready for review September 25, 2025 11:09
@GuySten GuySten changed the title Enable specifying reference direction for azimutal angle in PolarAzimuthal distribution Enable specifying reference direction for azimuthal angle in PolarAzimuthal distribution Sep 27, 2025
@shimwell
Copy link
Member

shimwell commented Oct 31, 2025

I noticed the point detector work would benefit from this PR and wondered if it is worth putting in a couple of tests. I can PR to your branch if that helps

def test_reference_vwu_projection():
    """When a non-orthogonal vector is provided, the setter should project out
    any component along reference_uvw so the stored vector is orthogonal.
    """
    pa = stats.PolarAzimuthal()  # default reference_uvw == (0, 0, 1)

    # Provide a vector that is not orthogonal to (0,0,1)
    pa.reference_vwu = (2.0, 0.5, 0.3)

    reference_v = np.asarray(pa.reference_vwu)
    reference_u = np.asarray(pa.reference_uvw)

    # reference_v should be orthogonal to reference_u
    assert abs(np.dot(reference_v, reference_u)) < 1e-6


def test_reference_vwu_normalization():
    """When a non-normalized vector is provided, the setter should normalize
    the projected vector to unit length.
    """
    pa = stats.PolarAzimuthal()  # default reference_uvw == (0, 0, 1)

    # Provide a vector that is neither orthogonal to (0,0,1) nor unit-length
    pa.reference_vwu = (2.0, 0.5, 0.3)

    reference_v = np.asarray(pa.reference_vwu)

    # reference_v should be unit length
    assert np.isclose(np.linalg.norm(reference_v), 1.0, atol=1e-12)

@GuySten
Copy link
Contributor Author

GuySten commented Oct 31, 2025

Thanks @shimwell. I invite you to push the tests to my PR.

@shimwell
Copy link
Member

Thanks @shimwell. I invite you to push the tests to my PR.

PR made GuySten#33

@GuySten
Copy link
Contributor Author

GuySten commented Nov 3, 2025

@shimwell, are you interested in reviewing this PR as well?

@shimwell
Copy link
Member

shimwell commented Nov 3, 2025

Yes sorry I did take a look at everything and those tests (thanks for fixing) were the only thing I thought it was missing. So this looks good to me. LGTM

@shimwell shimwell self-assigned this Nov 3, 2025
@shimwell shimwell self-requested a review November 3, 2025 06:15
@shimwell shimwell removed their assignment Nov 3, 2025
@GuySten GuySten merged commit fd964bc into openmc-dev:develop Nov 3, 2025
15 checks passed
@GuySten GuySten deleted the polar-fix branch November 3, 2025 07:43
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to chime in too late here -- the name reference_vwu doesn't really make sense to me. I'll submit a follow-on PR to suggest a new name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants